*: Support TIDB_VIEWS, TIDB_MLOGS and TIDB_TABLE_MVIEW_DEPENDENCIES in INFORMATION_SCHEMA for materialized view#68839
Conversation
ti-chi-bot
Bot
added
do-not-merge/invalid-title
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds three TiDB-specific INFORMATION_SCHEMA tables for materialized views, implements predicate extractors, executor data loaders with privilege/timezone handling, wires planner/builder, and provides unit and integration tests. ChangesMaterialized View INFORMATION_SCHEMA Support
Sequence Diagram(s)sequenceDiagram
participant Planner
participant MemtableRetriever
participant InfoSchemaTiDBMLogsExtractor as MLogExtractor
participant InfoSchema
participant PrivilegeManager
Planner->>MemtableRetriever: request memtable for tidb_mlogs
MemtableRetriever->>MLogExtractor: HasMLogPredicates()/ListSchemasAndTables...
MLogExtractor->>InfoSchema: list candidate tables
InfoSchema-->>MemtableRetriever: candidate table infos
MemtableRetriever->>PrivilegeManager: check privileges for MLOG and base table
PrivilegeManager-->>MemtableRetriever: allow/deny
MemtableRetriever->>Planner: emit rows for tidb_mlogs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @xzhangxian1008. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
xzhangxian1008
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/infoschema/tables.go`:
- Line 760: Change the new ID columns from VARCHAR to a numeric LONG type:
update the column definition for BASE_TABLE_ID (and the other new ID column
added nearby) to use mysql.TypeLonglong with size 20 and keep the
mysql.NotNullFlag (match the existing TIDB_MVIEWS.MVIEW_ID type/semantics);
ensure both places that currently set tp: mysql.TypeVarchar are changed to tp:
mysql.TypeLonglong and adjust size/flags accordingly so the INFORMATION_SCHEMA
ID fields are numeric and consistent.
In `@tests/integrationtest/t/executor/infoschema_mv.test`:
- Around line 45-51: The test currently snapshots raw MVIEW_MODIFY_TIME (queries
selecting MVIEW_MODIFY_TIME from information_schema.tidb_mviews), which bakes
wall-clock timestamps into infoschema_mv.result; update the test in
tests/integrationtest/t/infoschema_mv.test to stop capturing the raw
timestamp—either remove MVIEW_MODIFY_TIME from the SELECT projections or replace
that projection with a stable predicate/assertion (e.g., assert
MVIEW_MODIFY_TIME IS NOT NULL or use a deterministic flag column) in the queries
that reference MVIEW_MODIFY_TIME so the golden output is stable across
regenerations.
- Around line 13-15: The information_schema queries in
tests/integrationtest/t/executor/infoschema_mv.test are reading global state
(e.g., mv_t, mv_t1, mvv_t1, $mlog$t) which makes the golden output
order-dependent; modify the two queries that currently select from
information_schema.tidb_mviews and information_schema.tables to filter to the
test fixtures (for example by TABLE_SCHEMA = '<test_schema>' or by MVIEW_NAME IN
('mv_t','mv_t1','mvv_t1') / TABLE_NAME LIKE '$mlog$t%') so only objects created
by this test are counted, and add teardown steps at the end of the test to drop
mv_t, mv_t1, mvv_t1 and any $mlog$t artifacts (or create/use a dedicated schema
and drop it) so the test result is deterministic and stable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63a4fe24-61fc-46f6-a1dc-58bca52fcae4
📒 Files selected for processing (12)
pkg/executor/BUILD.bazelpkg/executor/builder.gopkg/executor/infoschema_reader.gopkg/executor/infoschema_reader_internal_test.gopkg/executor/test/ddl/materialized_view_ddl_test.gopkg/infoschema/tables.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/memtable_infoschema_extractor.gopkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazelpkg/planner/core/operator/logicalop/logicalop_test/logical_mem_table_predicate_extractor_test.gotests/integrationtest/r/executor/infoschema_mv.resulttests/integrationtest/t/executor/infoschema_mv.test
| select count(*) from information_schema.tidb_mviews; | ||
| select TABLE_SCHEMA, MVIEW_NAME from information_schema.tidb_mviews; | ||
|
|
There was a problem hiding this comment.
Scope these assertions to this file’s fixtures.
These queries read global INFORMATION_SCHEMA state, and this file never drops mv_t/mv_t1/mvv_t1 or $mlog$t. That makes the expected results suite-order dependent; Line 71 already has to weaken to count(*) >= 2 to tolerate leaked state. Filter by the objects created here (or use a dedicated schema) and clean them up at the end so the golden file stays stable. As per coding guidelines, tests/integrationtest/t/**: Integration test files changed: record and verify regenerated result correctness.
Also applies to: 39-40, 71-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrationtest/t/executor/infoschema_mv.test` around lines 13 - 15,
The information_schema queries in
tests/integrationtest/t/executor/infoschema_mv.test are reading global state
(e.g., mv_t, mv_t1, mvv_t1, $mlog$t) which makes the golden output
order-dependent; modify the two queries that currently select from
information_schema.tidb_mviews and information_schema.tables to filter to the
test fixtures (for example by TABLE_SCHEMA = '<test_schema>' or by MVIEW_NAME IN
('mv_t','mv_t1','mvv_t1') / TABLE_NAME LIKE '$mlog$t%') so only objects created
by this test are counted, and add teardown steps at the end of the test to drop
mv_t, mv_t1, mvv_t1 and any $mlog$t artifacts (or create/use a dedicated schema
and drop it) so the test result is deterministic and stable.
| select TABLE_SCHEMA, MVIEW_NAME, MVIEW_MODIFY_TIME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME = "mv_t" or MVIEW_NAME like "%mvv%" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME in ("mv_t", "mv", "mv_t1") order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME not in ("mv", "mv_t1") order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where TABLE_SCHEMA = "test" and MVIEW_NAME like "%mvv%" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where REFRESH_METHOD = "FAST" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, REFRESH_METHOD from information_schema.tidb_mviews where REFRESH_METHOD != "FAST" order by 1, 2, 3; | ||
| select TABLE_SCHEMA, MVIEW_NAME, MVIEW_MODIFY_TIME, REFRESH_METHOD from information_schema.tidb_mviews where MVIEW_NAME in ("mv_t", "mv", "mv_t1") and REFRESH_METHOD = "FAST" order by 1, 2, 3; |
There was a problem hiding this comment.
Don’t snapshot raw MVIEW_MODIFY_TIME in the golden output.
Projecting MVIEW_MODIFY_TIME here bakes wall-clock timestamps into infoschema_mv.result, so a clean regeneration will differ even when behavior is correct. Prefer a stable assertion such as MVIEW_MODIFY_TIME is not null or another predicate that does not depend on the creation timestamp. As per coding guidelines, tests/integrationtest/t/**: Integration test files changed: record and verify regenerated result correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrationtest/t/executor/infoschema_mv.test` around lines 45 - 51,
The test currently snapshots raw MVIEW_MODIFY_TIME (queries selecting
MVIEW_MODIFY_TIME from information_schema.tidb_mviews), which bakes wall-clock
timestamps into infoschema_mv.result; update the test in
tests/integrationtest/t/infoschema_mv.test to stop capturing the raw
timestamp—either remove MVIEW_MODIFY_TIME from the SELECT projections or replace
that projection with a stable predicate/assertion (e.g., assert
MVIEW_MODIFY_TIME IS NOT NULL or use a deterministic flag column) in the queries
that reference MVIEW_MODIFY_TIME so the golden output is stable across
regenerations.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view-2603 #68839 +/- ##
===============================================================================
Coverage ? 20.9831%
===============================================================================
Files ? 1683
Lines ? 634153
Branches ? 0
===============================================================================
Hits ? 133065
Misses ? 484043
Partials ? 17045
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/infoschema/infoschema_test.go (1)
236-245: ⚡ Quick winCover
TIDB_VIEWSin this smoke test.This block only exercises
TIDB_MLOGSandTIDB_TABLE_MVIEW_DEPENDENCIES, so a missinginfoschema.TableTiDBMViewsregistration would still passTestBasic. Add a simpleTableByNameassertion here (or inTestInfoTables) to keep this file aligned with the PR surface.🧪 Suggested patch
// Make sure partitions table exists tb, err = is.TableByName(context.Background(), pmodel.NewCIStr("information_schema"), pmodel.NewCIStr("partitions")) require.NoError(t, err) require.NotNil(t, tb) + tb, err = is.TableByName(context.Background(), pmodel.NewCIStr("information_schema"), pmodel.NewCIStr(infoschema.TableTiDBMViews)) + require.NoError(t, err) + require.NotNil(t, tb) tb, err = is.TableByName(context.Background(), pmodel.NewCIStr("information_schema"), pmodel.NewCIStr(infoschema.TableTiDBMLogs)) require.NoError(t, err)As per coding guidelines, "Test files: Prefer extending existing test suites and fixtures over creating new scaffolding; keep test changes minimal and deterministic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infoschema/infoschema_test.go` around lines 236 - 245, The test block only checks TableTiDBMLogs and TableTiDBTableMViewDependencies; add a similar smoke assertion for infoschema.TableTiDBMViews by calling is.TableByName(ctx, pmodel.NewCIStr("information_schema"), pmodel.NewCIStr(infoschema.TableTiDBMViews)), require.NoError(t, err) and require.NotNil(t, tb) (or check a known public column on that table) so the presence/registration of TableTiDBMViews is validated; locate the existing assertions using is.TableByName in the same test and insert this additional assertion there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/infoschema/infoschema_test.go`:
- Around line 236-245: The test block only checks TableTiDBMLogs and
TableTiDBTableMViewDependencies; add a similar smoke assertion for
infoschema.TableTiDBMViews by calling is.TableByName(ctx,
pmodel.NewCIStr("information_schema"),
pmodel.NewCIStr(infoschema.TableTiDBMViews)), require.NoError(t, err) and
require.NotNil(t, tb) (or check a known public column on that table) so the
presence/registration of TableTiDBMViews is validated; locate the existing
assertions using is.TableByName in the same test and insert this additional
assertion there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf70b79-1858-4dfa-8572-9011d1698d47
📒 Files selected for processing (4)
pkg/executor/infoschema_reader.gopkg/executor/infoschema_reader_internal_test.gopkg/infoschema/infoschema_test.gopkg/infoschema/tables.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/executor/infoschema_reader_internal_test.go
- pkg/infoschema/tables.go
| err error | ||
| ) | ||
| switch { | ||
| case (!hasMLogPredicates && !hasBasePredicates) || (hasMLogPredicates && !hasBasePredicates): |
There was a problem hiding this comment.
this case can be simplified as case !hasBasePredicates?
There was a problem hiding this comment.
this case can be simplified as
case !hasBasePredicates?
fixed
| switch { | ||
| case (!hasMLogPredicates && !hasBasePredicates) || (hasMLogPredicates && !hasBasePredicates): | ||
| schemas, tables, err = ex.ListSchemasAndTables(ctx, e.is) | ||
| case !hasMLogPredicates && hasBasePredicates: |
There was a problem hiding this comment.
can be simplified as case !hasMLogPredicates?
There was a problem hiding this comment.
can be simplified as
case !hasMLogPredicates?
fixed
| } | ||
|
|
||
| var baseInfo mlogBaseTableInfo | ||
| if i < len(baseInfos) { |
There was a problem hiding this comment.
I think you can construct baseInfos after L1709 so here we can use baseInfos[i] for all cases
There was a problem hiding this comment.
I think you can construct
baseInfosafter L1709 so here we can usebaseInfos[i]for all cases
refined
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@xzhangxian1008: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: close #68838
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests